-
Notifications
You must be signed in to change notification settings - Fork 690
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updates of the rule use_pam_wheel_group_for_su #10714
Updates of the rule use_pam_wheel_group_for_su #10714
Conversation
Hi @rumch-se. Thanks for your PR. I'm waiting for a ComplianceAsCode member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
fc3b154
to
90378e1
Compare
@@ -1,6 +1,6 @@ | |||
documentation_complete: true | |||
|
|||
prodtype: ubuntu2004,ubuntu2204 | |||
prodtype: ubuntu2004,ubuntu2204,sle12,sle15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prodtype isn't sorted which triggers the CI test.
@rumch-se Please remove the "Merge" commit from this PR. |
Hello @teacup-on-rockingchair Have a nice day |
Why would @teacup-on-rockingchair do that? This PR is from a branch in @rumch-se 's fork. |
Hello @jan-cerny, There are 4 commits (3 of them done by me and the last one done by @teacup-on-rockingchair). The last one has "Merge" in its description. Are you talking about the last commit, aren't you? Have a nice day |
fca182e
to
c2cb156
Compare
c2cb156
to
b3e29ad
Compare
Hello @jan-cerny and @teacup-on-rockingchair Have a nice day |
if [ $(getent group ${var_pam_wheel_group_for_su}) ]; then | ||
# group exists | ||
groupdel -f ${var_pam_wheel_group_for_su} | ||
fi | ||
groupadd -f ${var_pam_wheel_group_for_su} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this handled by ensure_pam_wheel_group_empty already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @wokis
1/The rule https://github.com/ComplianceAsCode/content/tree/master/linux_os/guide/system/accounts/accounts-restrictions/root_logins/ensure_pam_wheel_group_empty - does not have ansible part.
According to this PR I am covering this part.
2/The rule is a part of ubuntu profile, and I am contribute for SLE 12/15.
Have a nice day
Rumen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @wokis
1/The rule https://github.com/ComplianceAsCode/content/tree/master/linux_os/guide/system/accounts/accounts-restrictions/root_logins/ensure_pam_wheel_group_empty - does not have ansible part. According to this PR I am covering this part.
2/The rule is a part of ubuntu profile, and I am contribute for SLE 12/15.
As wokis mentioned, you should use the rule ensure_pam_wheel_group_empty
together with use_pam_wheel_group_for_su
otherwise you won't be checking through oval if the group is empty and the things you are adding to bash and ansible won't be needed here.
- name: {{{ rule_title }}} - Ensure group {{ var_pam_wheel_group_for_su }} is removed | ||
group: | ||
name: "{{ var_pam_wheel_group_for_su }}" | ||
state: absent | ||
|
||
- name: {{{ rule_title }}} - Ensure group {{ var_pam_wheel_group_for_su }} exist | ||
group: | ||
name: "{{ var_pam_wheel_group_for_su }}" | ||
state: present |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this handled by ensure_pam_wheel_group_empty already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use ensure_pam_wheel_group_empty
together with use_pam_wheel_group_for_su
Hello @dodys Have a nice day |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks for addressing our comments!
I will leave to @teacup-on-rockingchair to do the final review and merge it.
Code Climate has analyzed commit a6b6843 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 53.4% (0.0% change). View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description:
Rationale: